QPY 18: give the register/clbit payload a tag byte instead of a "\x00"-prefixed string - #16765
Conversation
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
|
@Mohamedma96 something went wrong in your commit history here, it looks like merge gone wrong. Could you clean up the PR history to only show your commits? |
Pulse gate calibrations were removed in Qiskit 2.0. Since then the CalibrationsPack field in the QPY circuit payload has always been written as an empty placeholder (num_cals=0). Version 18 removes it entirely. - Bump QPY_VERSION to 18 in qiskit/qpy/common.py - Gate CalibrationsPack field in QPYCircuit on version < 18 (binrw Option + if attribute covers both reader and writer) - Guard Python writer and reader paths with version < 18 / 5 <= version < 18 - Add regression test asserting v18 output is 2 bytes smaller than v17 - Add reno release note and Version 18 docstring section Signed-off-by: Elad Venezian <eladv@il.ibm.com>
QPY format versions 1–17 mistakenly serialised integer and float INSTRUCTION_PARAM values in little-endian byte order. Version 18 corrects this to big-endian, consistent with the rest of the format. The fix is fully backward-compatible: files written at version ≤ 17 are still read correctly via the LittleForV17AndBelow path. The Python read/write path (circuits.py) only handles versions ≤ 16 and is unchanged — the little-endian encoding there remains correct for those versions. Key implementation points: - New ValueEndian enum (Big / Little / LittleForV17AndBelow) replaces binrw::Endian at every QPY value call site; ValueEndian::resolve(version) is the single place where the version-to-endian mapping lives. - GenericValue::as_little_for_v17_and_below(&self, version) consolidates the remaining as_le() call sites that work directly with GenericValue. - Regression tests in test/python/qpy/test_v18_big_endian_params.py verify that v17 and v18 bytes differ for float params, for-loop integer lists, and switch-case labels, and that both versions round-trip correctly. Signed-off-by: Elad Venezian <eladv@il.ibm.com>
- Inline endian.resolve() at use sites in load_value instead of extracting an upfront variable - Update Version 18 docstring: remove backward-compat boilerplate, add mention of the endianness fix - Consolidate v18 tests into a single test_v18.py with one class (TestV17VsV18); drop round-trips already covered by test_roundtrip.py Signed-off-by: Elad Venezian <eladv@il.ibm.com>
- cargo fmt: reformat circuit_reader.rs, circuit_writer.rs, params.rs, py_methods.rs, value.rs (line-length wrapping) - black: remove extra blank line in test_v18.py - clippy: add #[allow(dead_code)] on ValueEndian::Little to silence the "variant never constructed" warning while keeping the variant for completeness Signed-off-by: Elad Venezian <eladv@il.ibm.com>
Signed-off-by: Elad Venezian <eladv@il.ibm.com>
The CI round-trip tests for QPY v18 were failing because the test framework was generating Python writer/reader combinations for v17+, which are intentionally unsupported — Rust is the only supported codec for those versions. Root cause: all_qpy_combinations() in test_roundtrip.py generated (v18, Python, Rust) and (v18, Rust, Python) test cases by patching QPY_RUST_WRITE_MIN_VERSION. The Python codec was never updated to handle the v18 big-endian encoding, so round-trips silently produced wrong values (e.g. integer 1 read back as 72057594037927936). Fix: - Filter out Python writer/reader combinations for versions >= QPY_RUST_WRITE_MIN_VERSION in all_qpy_combinations(), keeping Python only for v13-16 where it is the only option. - Add explicit QpyError assertions in write_circuit() and read_circuit() in binary_io/circuits.py: if the Python path is entered for a version that requires Rust, raise immediately with a clear message rather than silently producing corrupt output. This makes the version boundary self-documenting and prevents future regressions. Also fix test/python/qpy/test_v18.py license header (http -> https). Signed-off-by: Elad Venezian <eladv@il.ibm.com>
Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
a8432fb to
d922727
Compare
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
d922727 to
7dae5fe
Compare
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
gadial
left a comment
There was a problem hiding this comment.
Looks good; I added some minor comments.
| } | ||
|
|
||
| /// Position of `clbit` in the circuit being written, which is how both encodings identify it. | ||
| fn find_clbit_index(clbit: &ShareableClbit, qpy_data: &QPYWriteData) -> Result<Clbit, QpyError> { |
There was a problem hiding this comment.
These are all good; but let's check whether (a) there are already functions doing this, which I may have missed in the first place and (b) whether the QPY code has other places with this functionality which can also use these functions.
If you already looked into it - great!
There was a problem hiding this comment.
you are right, there is fn CircuitData::clbit_index and fn CircuitDatA::cregs_data(&self) that can help avoid going over the cregs with iterator and duplicate code, I saw another code duplication of getting the clbit index or the creg name and updated the code in expr.rs
| Both spellings live here even though ``qpy.load`` currently dispatches every payload from | ||
| version 13 on to the Rust reader: this is the reference implementation of the format, it is what | ||
| ``use_rust=False`` selects, and ``test/python/qpy/test_roundtrip.py`` runs it against the Rust | ||
| one at every version. A Python implementation that stopped at version 17 would read the newest |
There was a problem hiding this comment.
A Python implementation should not even try to open a QPY18 file (maybe we need to verify this is the case). I'm not sure we need this code at all, although since you already wrote it I don't object to keeping it.
There was a problem hiding this comment.
I agree, but can't the user force using python by setting use_rust = false?
There was a problem hiding this comment.
After removing the circuit read and write functions in #16701, we should have no user-facing use_rust parameter. Basically, whenever the user runs qpy it should always use rust unless the read or write version is too small, and then it falls back to Python.
| The ``type`` field can be ``'i'``, ``'f'``, ``'p'``, ``'e'``, ``'s'``, ``'c'``, | ||
| ``'R'`` or ``'n'`` which dictate the format. ``'R'`` is a `REGISTER_PARAM` payload, | ||
| identifying a :class:`.ClassicalRegister` or a single :class:`.Clbit` in the encoding described in | ||
| :ref:`version 18 <qpy_version_18>`. For ``'i'`` it's an integer, ``'f'`` it's |
There was a problem hiding this comment.
This is a description of QPY17; we should not mention any QPY18 format here.
| qc.if_else(condition, body, false_body, [qc.qubits[0]], []) | ||
| self.assert_roundtrip_equal(qc, version=version, read_with=read_with, write_with=write_with) | ||
|
|
||
| @all_qpy_combinations(QPY_RUST_READ_MIN_VERSION) |
There was a problem hiding this comment.
Roundtrip tests should not cover QPY18 features which are not supposed to be handled in Python. We do need to test the new features; the #16707 PR added test/python/qpy/test_v18.py for that purpose.
There was a problem hiding this comment.
moved the tests to test_v18.py and removed the tests from round_trip.py
Signed-off-by: mohamedmahameed <mohamed.mahameed@ibm.com>
QPY values of type 'R' can represent either a ClassicalRegister or a single Clbit.
QPY ≤17: both use the existing string-based encoding, including the \x00 prefix hack for clbits.
QPY 18+: introduce an explicit tagged binary representation: Clbit uses a tag + uint32 index, while ClassicalRegister uses a tag + name.
Added a binrw enum in Rust and matching Python serialization/deserialization.
Added round-trip tests for single-clbit conditions across QPY versions and Rust/Python readers/writers.
Legacy QPY ≤17 encoding remains unchanged.
built on top of this PR:
#16707
which already made the changes needed for pumping up the qpy version.
Relevant commit to review:
(Last commit) - 7dae5fe
AI/LLM disclosure
Claude Code (Anthropic), version